HIVE-28400: Refactor QueryProperties feature flags to use QueryFeature enum#6560
HIVE-28400: Refactor QueryProperties feature flags to use QueryFeature enum#6560Manya0407 wants to merge 1 commit into
Conversation
Manya0407
changed the title
|
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
Thanx @Manya0407, Changes LGTM.
The original ticket was from @zabetak , Will be good to get his blessing as well. I am even thinking should we deprecate the individual methods and recommend using the new one quoting these individual methods will be dropped in the subsequent release
abstractdog
left a comment
abstractdog
left a comment
There was a problem hiding this comment.
left minor comments
I found it hard to decide which boolean fields fall into the "feature" category and which don't, now I feel all of them are features except the basic ones like
boolean query;
boolean analyzeCommand;
boolean noScanAnalyzeCommand;
boolean analyzeRewrite;
boolean ctas;
| private boolean hasLateralViews = false; | ||
| private boolean cboSupportedLateralViews = true; |
There was a problem hiding this comment.
these seem to be features too
| return features.contains(feature); | ||
| } | ||
|
|
||
| public boolean hasGroupBy() { |
There was a problem hiding this comment.
the jira description says "the class is unnecessarily verbose since each feature requires a dedicated getter/setter"
I feel we can go on with removing these without deprecation
There was a problem hiding this comment.
Agree, it would be nice to get rid of the setters/getters. It's a rather internal class so I don't think we need to deprecate first.
| private boolean multiDestQuery; | ||
| private boolean filterWithSubQuery; |
There was a problem hiding this comment.
these seem to be features too
| private boolean isMaterializedView; | ||
| private boolean isView; |
There was a problem hiding this comment.
these seem to be features too
| return features.contains(feature); | ||
| } | ||
|
|
||
| public boolean hasGroupBy() { |
There was a problem hiding this comment.
Agree, it would be nice to get rid of the setters/getters. It's a rather internal class so I don't think we need to deprecate first.
| boolean hasDistributeBy = false; | ||
| boolean hasClusterBy = false; | ||
| private final EnumSet<QueryFeature> features = EnumSet.noneOf(QueryFeature.class); | ||
| boolean mapJoinRemoved = false; |
There was a problem hiding this comment.
This seems to be unused so it can be dropped; it is set but never queried/accessed. Check if there are other unused feature/flags and remove them as well.
| boolean analyzeCommand; | ||
| boolean noScanAnalyzeCommand; | ||
| boolean analyzeRewrite; | ||
| boolean ctas; |
There was a problem hiding this comment.
I would put CTAS, and ANALYZE as feature as wells. For the mutliple analyze variants maybe we could use multiple enum entries:
analyzeCommand -> QueryFeature.ANALYZE
noScanAnalyze -> QueryFeature.ANALYZE + QueryFeature.NO_SCAN
analyzeRewrite -> QueryFeature.ANALYZE + QueryFeature.REWRITE
| boolean ctas; | ||
| int outerQueryLimit; | ||
|
|
||
| boolean hasJoin = false; |
5 participants
What changes were proposed in this pull request?
1.This PR refactors
QueryPropertiesfeature tracking from multiple boolean fields to an enum-based model.2.Query feature flags such as
GROUP_BY,ORDER_BY,LIMIT,PTF,WINDOWING,QUALIFY,EXCEPT,INTERSECT,USES_SCRIPT, and related clause flags are now represented byQueryFeaturevalues stored in anEnumSet<QueryFeature>.3.Relevant feature writer and reader call sites were updated to use
addFeature(QueryFeature.X)andhasFeature(QueryFeature.X).4.Compatibility wrapper methods such as
hasGroupBy()andsetHasGroupBy(boolean)continue to delegate to the enum-backed storage. Non-feature state such as join counts, query type, lateral view flags, view/materialized view flags, analyze flags, and used tables remains unchanged.Why are the changes needed?
The previous implementation stored query features as many individual boolean fields, which made
QueryPropertiesharder to read and extend.Using aQueryFeatureenum withEnumSetkeeps related feature state in one place, reduces repeated boolean field management, and makes adding future query features simpler and less error-prone.EnumSetis also efficient for enum values and is a good fit for representing query features.Does this PR introduce any user-facing change?
No
How was this patch tested?
Manual testing